Fix: Move replica validation logic to right place. #4307
Conversation
kash2104
requested review from
MortalHappiness,
andrewsykim,
kevin85421 and
rueian
as code owners
kash2104
force-pushed
the
issue-4101
branch
3 times, most recently
from
d1d7b57 to
eb654d7
Compare
|
Thanks for your effort. cc @Future-Outlier |
|
I have gone through #4116 but it was inactive and no changes were being made after the merging of volcano pr. Many changes are required since volcano pr's merging, that's why I opened up this PR. |
|
Gotcha. Let's wait for maintainers' reply. I'll help review, thank you. |
|
@Future-Outlier Just wanted to follow up - do you prefer closing #4116 and continuing the work in this PR? |
Hi, @kash2104 I just left a comment to #4116 and ask her if she have time to finish the work. |
kash2104
force-pushed
the
issue-4101
branch
3 times, most recently
from
529b021 to
bc5decd
Compare
Future-Outlier
moved this from In Progress
to to review
in @Future-Outlier's kuberay project
Future-Outlier
left a comment
Future-Outlier
left a comment
There was a problem hiding this comment.
cc @400Ping @justinyeh1995 @machichima to review
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if *workerGroup.MinReplicas > *workerGroup.MaxReplicas { | ||
| return fmt.Errorf("worker group %s has minReplicas %d greater than maxReplicas %d", workerGroup.GroupName, *workerGroup.MinReplicas, *workerGroup.MaxReplicas) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we also check if workerGroup.Replicas lies within the workerGroup.MinReplicas and workerGroup.MaxReplicas`?
There was a problem hiding this comment.
@machichima Here in thevalidation.go, we are just checking whether the values for min and max replicas aren't incorrect or impossible before pod creation happens but the actual logic of number of replicas is moved to util.go
So I think that here in validation.go, we won't be needing this check.
| if nodeGroup.MinReplicas != nil { | ||
| minReplicas = *nodeGroup.MinReplicas | ||
| } |
There was a problem hiding this comment.
Maybe we can use ptr.Deref, which is used in other places as well
kuberay/ray-operator/controllers/ray/utils/util.go
Lines 830 to 831 in 270a143
1. Remove validation logic from GetWorkerGroupDesiredReplicas (utils.go) and add this logic to ValidateRayClusterSpec (validation.go). 2. Remove unnecessary tests from TestGetWorkerGroupDesiredReplicas. 3. Remove the unused ctx.
This is added since we moved the validation logic.
5 participants
Why are these changes needed?
These changes are done to check the validation logic before rayCluster pod creation. It moves the replica validation logic as well as removes the redundant tests. Along with this, unit test are added since we moved the logic from
utils.gotovalidation.go.Related issue number
Closes #4101
Checks